Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ability to set SmartLifecycle.phase to SqsMessageListenerContainer/DefaultListenerContainerRegistry #821

Merged
merged 7 commits into from
Jun 26, 2023
Merged

Ability to set SmartLifecycle.phase to SqsMessageListenerContainer/DefaultListenerContainerRegistry #821

merged 7 commits into from
Jun 26, 2023

Conversation

estigma88
Copy link
Contributor

@estigma88 estigma88 commented May 18, 2023 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

  • Adding the ability to set the SmartLifecycle.phase on SqsMessageListenerContainer using the builder
  • Adding the ability to set the SmartLifecycle.phase on AbstractMessageListenerContainer using the setPhase method
  • Adding the ability to set the SmartLifecycle.phase on DefaultListenerContainerRegistry using the setPhase method
  • Update unit tests on SqsMessageListenerContainerTests, AbstractMessageListenerContainerTests and DefaultListenerContainerRegistryTests

💡 Motivation and Context

Details on #693

💚 How did you test it?

  • Update unit tests on SqsMessageListenerContainerTests and AbstractMessageListenerContainerTests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions github-actions bot added the component: sqs SQS integration related issue label May 18, 2023
tomazfernandes
tomazfernandes requested changes May 18, 2023
View reviewed changes
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @estigma88 , thanks! Just one minor comment.

tomazfernandes
tomazfernandes approved these changes May 20, 2023
View reviewed changes
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR @estigma88!

Looking forward to more, there are quite a few open SQS issues, if you see any that you'd like to contribute just let us know.

@maciejwalkowiak
Copy link
Contributor

Perhaps we should update reference docs with an explanation?

@tomazfernandes
Copy link
Contributor

Perhaps we should update reference docs with an explanation?

Good call @maciejwalkowiak.

@estigma88, would you mind adding a few lines to the doc regarding phase? Let me know if you need any help with that.

Thanks!

@estigma88
Copy link
Contributor Author

estigma88 commented May 22, 2023 •
edited
Loading

Sure @maciejwalkowiak @tomazfernandes , I will add some docs.

Btw, I was checking a little deep over the sqs framework, and I found that DefaultListenerContainerRegistry also implements SmartLifeCycle, seems like there are different ways to initialize listeners in the framework, should I add the ability to set the phase there also? I think the PR is incomplete

@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label May 23, 2023
@tomazfernandes
Copy link
Contributor

@estigma88 sorry, I’m in the middle of lots of things here and couldn’t reply before.

It makes sense to update the DefaultListenerContainerRegistry, good catch. Please add it to the PR if you can.

Thanks.

@estigma88 estigma88 changed the title Ability to set SmartLifecycle.phase to SqsMessageListenerContainer Ability to set SmartLifecycle.phase to SqsMessageListenerContainer/DefaultListenerContainerRegistry May 23, 2023
@estigma88
Copy link
Contributor Author

@tomazfernandes don't worry.

I updated the docs, let me know if that makes sense, also, did a little refactor to reuse the DEFAULT_PHASE in the DefaultListenerContainerRegistry and being able to change it easily in the future.

tomazfernandes
tomazfernandes requested changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estigma88 , just two comments about the documentation changes, please let me know if you have any questions.

Thanks.

docs/src/main/asciidoc/sqs.adoc Outdated
@@ -927,6 +927,8 @@ The `MessageListenerContainer` interface extends `SmartLifecycle`, which provide
Containers created from `@SqsListener` annotations are registered in a `MessageListenerContainerRegistry` bean that is registered by the framework.
The containers themselves are not Spring-managed beans, and the registry is responsible for managing these containers` lifecycle in application startup and shutdown.

NOTE: The framework offers the `DefaultListenerContainerRegistry` implementation, where you can override the `SmartLifecycle.phase`. By default, the `phase` value is `MessageListenerContainer.DEFAULT_PHASE`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We try not to use you in the documentation, and adopt a more formal / neutral tone.

How about something along these lines:

Suggested change
NOTE: The framework offers the `DefaultListenerContainerRegistry` implementation, where you can override the `SmartLifecycle.phase`. By default, the `phase` value is `MessageListenerContainer.DEFAULT_PHASE`
NOTE: The `DefaultListenerContainerRegistry ` implementation provided by the framework allows the phase value to be set through the `setPhase` method. The default value is `MessageListenerContainer.DEFAULT_PHASE`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

docs/src/main/asciidoc/sqs.adoc Outdated
@@ -955,6 +957,8 @@ MessageListenerContainer<Object> listenerContainer(SqsAsyncClient sqsAsyncClient
}
----

NOTE: You can also specify the `SmartLifecycle.phase`, using `SqsMessageListenerContainer.builder()`, if you are interested on overriding the default value defined in `MessageListenerContainer.DEFAULT_PHASE`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about using you. Please try to rewrite using the passive voice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

tomazfernandes
tomazfernandes approved these changes Jun 25, 2023
View reviewed changes
Copy link
Contributor

@tomazfernandes tomazfernandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @estigma88! Looking forward to more!

@tomazfernandes tomazfernandes added type: feature Integration with a new AWS service or bigger change in existing integration and removed type: documentation Documentation or Samples related issue labels Jun 26, 2023
@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label Jun 26, 2023
@tomazfernandes tomazfernandes merged commit 42da25f into awspring:main Jun 26, 2023
4 checks passed
@tomazfernandes tomazfernandes mentioned this pull request Jun 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomazfernandes tomazfernandes tomazfernandes approved these changes

@maciejwalkowiak maciejwalkowiak Awaiting requested review from maciejwalkowiak maciejwalkowiak is a code owner

@MatejNedic MatejNedic Awaiting requested review from MatejNedic MatejNedic is a code owner

Assignees
No one assigned
Labels
component: sqs SQS integration related issue type: documentation Documentation or Samples related issue type: feature Integration with a new AWS service or bigger change in existing integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@estigma88 @maciejwalkowiak @tomazfernandes